@yusuf-musleh I think your best approach would be to modify (and rename) TagListPermissions to provide a perm_map like we did with ObjectTagObjectPermissions.

That will enforce the oel_tagging.view_tag, oel_tagging.change_tag, and oel_tagging.delete_tag rules before invoking the view methods.

But I think you're correct in your question -- these oel_tagging.*_tag rules need to be updated to reflect the changes to the taxonomy view/change rules made by #94.

I think something like this would be sufficient?

def can_view_tag(user: UserType, tag: Tag | None = None) -> bool:
    """
    Users can view tags for any taxonomy they can view.
    """"
    taxonomy = tag.taxonomy.cast() if (tag and tag.taxonomy) else None
    return  user.has_perm(
        "oel_tagging.view_taxonomy",
        taxonomy,
    )

def can_change_tag(user: UserType, tag: Tag | None = None) -> bool:
    """
    Users can change tags for any taxonomy they can modify.
    """"
    taxonomy = tag.taxonomy.cast() if (tag and tag.taxonomy) else None
    return  user.has_perm(
        "oel_tagging.change_taxonomy",
        taxonomy,
    )

rules.add_perm("oel_tagging.add_tag", can_change_tag)
rules.add_perm("oel_tagging.change_tag", can_change_tag)
rules.add_perm("oel_tagging.delete_tag", can_change_tag)
rules.add_perm("oel_tagging.view_tag", can_view_tag)
rules.add_perm("oel_tagging.list_tag", can_view_tag)

@pomegranited Thanks for the pointers! I went ahead with what you suggested, however I faced a few issues and took me quite some time to figure them out:

1. The get_queryset method in TaxonomyTagsView was initially returning a list, as it does some custom population of an attribute subtags and later serialized using the TagsForSearchSerializer. For the updated permissions (using perm_map) to work,get_queryset needs to return a queryset as it is used internally by DRF. The current solution I have (though not a huge fan of it) is getting the result list and constructing a queryset using filter and then repopulating the subtags again if applicable. Ideally, I think the approach should be to refactor the underlying implementation of get_matching_tags to use querysets instead. I couldn't get to that today, but I thought I'd mention it here to get your thoughts.
2. After adding the perms_map for TagObjectPermissions, I left the has_object_permission method in place, since it's a special case for the GET list endpoint. There is not tag object in that case, we have the Taxonomy object, so we utilize the can_view_taxonomy permission for the oel_tagging.list_tag case. It gets called in the get_taxonomy method to and handle whether the use can view the Tags under this Taxonomy or not.

@yusuf-musleh

1... For the updated permissions (using perm_map) to work, get_queryset needs to return a queryset as it is used internally by DRF... Ideally, I think the approach should be to refactor the underlying implementation of get_matching_tags to use querysets instead.

I agree.. have you seen @bradenmacdonald 's #92 ? I think that PR does what you need already. We ran out of budget to complete it, but if it's needed for this work, then maybe we can complete it here?

And what you did for the permissions looks great, thank you.

@pomegranited I went through @bradenmacdonald 's #92, lots of great work there, and already does alot of the heavy lifting for the refactoring, however since there are substantial changes made in that PR (and some more work remaining there) I feel it might get pretty messy to include it as part of this PR. Would it make sense to leave the current code in this PR as is and then continue working on #92 and along with the refactoring needed here as part of a separate ticket?

Alternatively, I could copy the relevant changes from that #92 and include it as part of this PR? What do you guys think?

Would it make sense to leave the current code in this PR as is and then continue working on #92 and along with the refactoring needed here as part of a separate ticket?

Yes, I think that makes sense :)

Created openedx/modular-learning#142 for this.

Great, thanks for creating it @pomegranited !

I would generally prefer that we use value to identify tags in these API methods, rather than having an int tag ID. Same for parent_tag_id -> parent_tag_value and tag_ids -> list[str].

The reasons are:

• In Simplify Tag Models [FC-0030] #87 I found that using value (which the DB enforces to be a unique ID) lets us handle free-text and closed taxonomies consistently, without need different identifiers for different types of taxonomies. This reduced the overall line count and made it simpler to reason about.
• This lets us use a similar argument type (value) for add_tag_to_taxonomy and update_tag_in_taxonomy
• The tag_object API now uses value to identify tags, so I think we should be consistent with that.
• My proposed optimization/simplification of getting tags emphasizes value over ID, and I'm still hoping I'll be able to finish that up at some point.
• IDs aren't necessarily stable in the test suite, but value is so you can simplify your tests

But I know there are downsides as well so let me know if you guys don't like that direction.

Ooh good points, @bradenmacdonald -- I agree that using value where we can makes sense.

I see, that makes sense, I updated the implementation to use values instead of IDs. 3abce26

I don't understand why this validation needs to be this complicated, when the same validation on the Taxonomy.delete_tags is much simpler?

Also, do we need to this extra validation in these serializers, since the implementation also enforces the same rules? I'm genuinely asking, I'm not sure where this should sit, but I don't think it needs to be duplicated.

The implementation in the Taxonomy.delete_tags is actually an incorrect one, since the simple value__in filter doesnt account for case-insensitve values, the current tests didn't catch it. Thanks for pointing it out, I'll update it.

Also, do we need to this extra validation in these serializers, since the implementation also enforces the same rules?

My initial thought process was to add the validation in the api incase the functions were called directly rather than only in the rest_api/views (that go through serializers), however then I figured we could also make use of the validation that DRF provides in addition to that.

The rules they enforce are slightly different, you'll notice the validation that happens in the serializer checks if the Tag values provided are valid (i.e exist in the DB regardless to which taxonomy they belong to) however the validation that happens in the api method checks if the Taxonomy has the provided (Tag) values, slight difference where the former would return a 400 and the later would return a 404. I guess the end result is technically the same (the request fails), just thought this give more granularity in checks and errors.

The implementation in the Taxonomy.delete_tags is actually an incorrect one, since the simple value__in filter doesnt account for case-insensitve values, the current tests didn't catch it.

Actually, doing some tests, surprising the simple implementation works 😮, It might be because of the special custom case_insensitive_char_field we use in the model? Since that is generally not the default behavior for django's charfields. In any case, that was a pleasant surprise! I went ahead and simplified the validation.

Ah lovely, I'm glad to hear that case-insensitive fields aren't so hard to interact with!

I'm still not convinced that the extra Serializer validation is needed, since the model does what it does and more. But I'll leave that to you & upstream's discretion.

Personally I would put this validation in the Taxonomy.add_tag method, where most other validation is anyways. That way it gets used for both the python API and REST API. Currently, only the REST API has this validation. You can leave it as-is though.

Removing the validation method(s) from these three serializers would also make the serializer code much more compact.

After trying it out, it's definitely more concise and having the validations all in one place is cleaner, thanks @pomegranited and @bradenmacdonald for the suggestions! I updated it. b0e4b26

Nit: I think it would be a bit more RESTful to put the old value in the URL and just use value as the name of the new value field. Though since value is a mutable ID it's not a huge deal. So no need to change for now.

What I would definitely recommend we do now is remove the PUT example from the docstring. Because if we add the ability to change other fields like external_id via this API endpoint in the future, anyone who is using PUT to change the name will now be accidentally overwriting/deleting the external_id, since PUT means "completely replace the tag with this new spec". But PATCH explicitly means "update the fields I specify to the new values and leave others unchanged".

I see, for sure, I removed the PUT example as suggested, and kept the endpoint as is for now.